cache the collection schema, only read from vatstore once per crank #7333

warner · 2023-04-05T06:41:59Z

This changes the collectionManager to keep the per-collection
metadata (keyShape, valueShape, label) in a cache. Like the VOM's
dataCache, this schemaCache is populated at most once per crank, and
flushed at the end of every crank. This removes the GC-sensitive
syscalls that used to fetch the metadata when userspace caused a
collection to be reanimated. Instead, the cache is read when userspace
invokes a collection method that needs the data (most of them, to
validate keys or values), regardless of whether the collection
Representative remains in RAM or not.

It also removes the now-unused in-RAM list of all collections, which used to support deleteAllVirtualCollections, called from stopVat.

closes #6360
refs #5058
closes #7142

warner · 2023-04-05T06:51:02Z

@FUDCo the last commit changes the serialized |schemata value from [keyShape, valueShape] to { keyShape, valueShape } (from array to object/record). I thought that make future extensions more straightforward, but I could be talked out of it, especially since it grows the stored size slightly (it's storing an explicit undefined for both, but maybe we should change that, by building up the serialized object one property at a time). I'm moderately sure that there's no functional difference between e.g. { keyShape, valueShape: undefined } and just { keyShape }.

So two questions for you to think about:

avoid storing values of undefined in the record?
record vs array?

A performance note: this approach does either zero or one reads of |schemata and |label per collection per crank, triggered by pretty much any collection method (since nearly all of them want to validate against keyShape). The schemata it reads is flushed at end-of-crank. This makes the syscall trace deterministic, but I'd respect an argument that once-per-crank is too frequent. The next most logical thing to do would be to hold them across multiple cranks, until a BOYD flushes the cache. That frequency felt too slow (i.e. too high RAM usage) for the VOM dataCache, but the virtual-collection schemaCache holds a lot less data, so we might choose a different tradeoff.

@mhofman this ought to remove the gc-sensitivity due to collections being collected and then reanimated. And #7138 removed another. I think there's one known source of sensitivity for me to try to fix, involving KindHandles being reanimated (the other part of #7142).

warner · 2023-04-05T07:18:03Z

oops, something is still broken, will re-submit when ready

warner · 2023-04-05T08:30:49Z

Ok I went ahead and changed the serialized schemata object to be empty if there were no keyShape/valueShape constraints given. In practice, there's always a keyShape, because the default is M.scalar(), but this still saves some space in the serialized form when valueShape = undefined (which is really common)

I'm storing schemataCD (capdata) in the cache. This isn't obviously useful for this PR, but there's another one stacked up on top of it (#7334, for #7321) that needs the .slots to do refcounting on.

mhofman · 2023-04-05T09:02:38Z

I went ahead and changed the serialized schemata object to be empty if there were no keyShape/valueShape constraints given

Quick question: is there any reason not to merge the schemata and label vatStore entries into a single one? Especially now that we have a record for the schemata.

warner · 2023-04-05T15:09:12Z

I went ahead and changed the serialized schemata object to be empty if there were no keyShape/valueShape constraints given

Quick question: is there any reason not to merge the schemata and label vatStore entries into a single one? Especially now that we have a record for the schemata.

I think that'd be fine. We've got two mutable metadata keys (|entryCount, updated each time we add/remove something from a collection, and |nextOrdinal, incremented when we add a new vref-based key), and two immutable ones (|label and |schemata). We'll want to keep the mutable ones as they are, they have different "temperatures". But merging all the immutables ones makes sense, and then naming them something closer to |info.

@FUDCo can you remember why they were split?

FUDCo · 2023-04-05T17:47:35Z

@FUDCo can you remember why they were split?

I don't think there was a deep reason. Mainly, I think, to avoid having to parse the value to separate two relatively unrelated pieces of information from each other.

warner · 2023-04-05T17:50:01Z

Cool, thanks. Ok, now that we're reading them both at the same time (schemata, to build the keyShape/valueShape checkers, and label, to build the error messages emitted if those checkers fail), it'll save a vatstore read to merge them together. I'll add a commit to do that.

FUDCo

I think this is OK, but there are a few points I'm unclear on that I'd like to be clear on before approving.

packages/swingset-liveslots/src/collectionManager.js

FUDCo · 2023-04-06T20:54:44Z

packages/swingset-liveslots/src/collectionManager.js

+    return harden({ keyShape, valueShape, label, schemataCD });
+  };
+  /** @type {(collectionID: string, value: SchemaCacheValue) => void } */
+  const writeBacking = (collectionID, value) => {
+    const { label, schemataCD } = value;
+    const schemataKey = prefixc(collectionID, '|schemata');
+    const schemataValue = JSON.stringify(schemataCD);


Why return keyShape, valueShape, AND schemataCD from the read but stringify schemataCD in the write? If the consumer needs the shapes, having the schemataCD doesn't help, and when writing the producer (presumably) generates the keyShape and valueShape values and shouldn't concerned with how they're encoded. Am I missing something here?

This is preparation for #7321 (PR #7334), which needs to know about any slots (vrefs) in the serialized keyShape/valueShape. When the collection is deleted, rather than re-serialize those values (to learn the vrefs, so we can feed them to vrm.removeReachableVref, to decrement their refcounts), we keep the original serialized forms around. The memory overhead of retaining the serialized capdata is pretty small, and it didn't occur to me to trust that serialization would return the same values as the original.. felt more appropriate to retain the original instead.

The only time we write to the schemaCache is when a collection is first created, and at that time we also have the serialized capdata available (again, as anticipation for #7334, which needs to increment refcounts for everything therein).

packages/swingset-liveslots/src/collectionManager.js

packages/swingset-liveslots/src/liveslots.js

FUDCo

Well OK then

This removes `deleteAllVirtualCollections`, and the in-RAM `allCollectionObjIDs` Set of all collectionIDs which supported it. Once upon a time, `stopVat` called `deleteAllVirtualCollections` to delete the vatstore data for all non-durable virtual collections. We removed responsibility for deleting virtual data from the vat (and stopped calling `stopVat` altogether) in the interests of reducing upgrade risk, and improving upgrade performance, despite the fact that this will cause a DB storage leak during upgrade (we figure we can find a way to fix that later, and disk is cheap). Now that `deleteAllVirtualCollections` is gone, we don't need to spend the RAM on each collection, which violated our high-cardinality design rules anyways. refs #5058

This changes the collectionManager to keep the per-collection metadata (keyShape, valueShape, label) in a cache. Like the VOM's dataCache, this `schemaCache` is populated at most once per crank, and flushed at the end of every crank. This removes the GC-sensitive syscalls that used to fetch the metadata when userspace caused a collection to be reanimated. Instead, the cache is read when userspace invokes a collection method that needs the data (most of them, to validate keys or values), regardless of whether the collection Representative remains in RAM or not. It also changes the serialized form of the schemata to be an object like `{ keyShape, valueShape }` instead of an array like `[keyShape, valueShape]`. If the constraints are missing, the object is empty, which is smaller to serialize. I'm also thinking this might make it more extensible. closes #6360

Virtual collections need to record a label and a keyShape/valueShape schema. Previously these were kept in two separate vatstore keys, `|label` and `|schemata`. With this change, they're both stored in the `|schemata` key. This should reduce the number of vatstore reads necessary to work with a collection.

warner added SwingSet package: SwingSet liveslots requires vat-upgrade to deploy changes labels Apr 5, 2023

warner requested a review from FUDCo April 5, 2023 06:41

warner self-assigned this Apr 5, 2023

warner marked this pull request as draft April 5, 2023 07:17

warner force-pushed the 6360-schema-cache branch from 10cc46d to 91f525f Compare April 5, 2023 08:28

warner marked this pull request as ready for review April 5, 2023 08:34

FUDCo reviewed Apr 6, 2023

View reviewed changes

warner mentioned this pull request Apr 7, 2023

liveslots double-free of virtual objects/collections when RAM and vdata pillars drop on same BOYD #7353

Closed

warner changed the base branch from master to 7353-double-free April 7, 2023 05:57

warner force-pushed the 6360-schema-cache branch from 91f525f to 2685344 Compare April 7, 2023 05:57

warner requested a review from FUDCo April 7, 2023 06:53

warner mentioned this pull request Apr 7, 2023

GC syscall sensitivity through reanimate and reanimateDurableKindID #7142

Closed

warner force-pushed the 7353-double-free branch from 6133932 to 0dc663e Compare April 9, 2023 03:34

warner force-pushed the 6360-schema-cache branch from edcb932 to 082f244 Compare April 9, 2023 03:34

warner mentioned this pull request Apr 9, 2023

Add APIs for tracking/debugging undesired object retention (aka "leaks") #7362

Merged

FUDCo approved these changes Apr 10, 2023

View reviewed changes

warner force-pushed the 6360-schema-cache branch from 082f244 to 83155ef Compare April 10, 2023 20:49

warner force-pushed the 7353-double-free branch 2 times, most recently from 49c6e6b to 8b58027 Compare April 10, 2023 22:57

warner force-pushed the 6360-schema-cache branch from 83155ef to 8a3ccc0 Compare April 10, 2023 22:57

warner force-pushed the 7353-double-free branch from 8b58027 to d5114d2 Compare April 10, 2023 23:17

warner force-pushed the 6360-schema-cache branch from 8a3ccc0 to 09a7d00 Compare April 10, 2023 23:17

warner force-pushed the 7353-double-free branch from d5114d2 to 8316eb9 Compare April 11, 2023 01:19

Base automatically changed from 7353-double-free to master April 11, 2023 01:58

warner added 6 commits April 10, 2023 19:04

liveslots: flush all caches after BOYD, not just stateCache

5a4ddb3

cache: assert that all keys are strings

ee0128d

be consistent about collectionID being a string

39eb8d5

warner force-pushed the 6360-schema-cache branch from 09a7d00 to 2faa0a3 Compare April 11, 2023 02:06

warner added force:integration Force integration tests to run on PR automerge:rebase Automatically rebase updates, then merge labels Apr 11, 2023

mergify bot merged commit 24de1b6 into master Apr 11, 2023

mergify bot deleted the 6360-schema-cache branch April 11, 2023 02:48

warner mentioned this pull request Apr 12, 2023

change virtual-object state LRU cache: write-through, write-back at end-of-crank, or remove entirely #6693

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cache the collection schema, only read from vatstore once per crank #7333

cache the collection schema, only read from vatstore once per crank #7333

warner commented Apr 5, 2023 •

edited

Loading

warner commented Apr 5, 2023

warner commented Apr 5, 2023

warner commented Apr 5, 2023 •

edited

Loading

mhofman commented Apr 5, 2023

warner commented Apr 5, 2023

FUDCo commented Apr 5, 2023

warner commented Apr 5, 2023

FUDCo left a comment

FUDCo Apr 6, 2023

warner Apr 7, 2023

FUDCo left a comment

cache the collection schema, only read from vatstore once per crank #7333

cache the collection schema, only read from vatstore once per crank #7333

Conversation

warner commented Apr 5, 2023 • edited Loading

warner commented Apr 5, 2023

warner commented Apr 5, 2023

warner commented Apr 5, 2023 • edited Loading

mhofman commented Apr 5, 2023

warner commented Apr 5, 2023

FUDCo commented Apr 5, 2023

warner commented Apr 5, 2023

FUDCo left a comment

Choose a reason for hiding this comment

FUDCo Apr 6, 2023

Choose a reason for hiding this comment

warner Apr 7, 2023

Choose a reason for hiding this comment

FUDCo left a comment

Choose a reason for hiding this comment

warner commented Apr 5, 2023 •

edited

Loading

warner commented Apr 5, 2023 •

edited

Loading